-
-
Notifications
You must be signed in to change notification settings - Fork 662
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[12.0][IMP] hr_holidays_leave_auto_approve: Auto-approve for everyone #663
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
8c4d92a
to
edc3464
Compare
This PR has the |
@CasVissers I'm sure you would be interested in this PR! |
@OCA/human-resources-maintainers Can this be merged? |
One more review needed |
@alexey-pelykh Isnt 2 the minimum? |
2 + 1 PCS |
Oh okay! |
/ocabot merge |
This PR looks fantastic, let's merge it! |
I'd also propose for next migration or improvement make a setting field of "auto-approve policy" instead of 2 Boolean fields |
@gurneyalex your merge command was aborted due to failed check(s), which you can inspect on this commit of 12.0-ocabot-merge-pr-663-by-gurneyalex-bump-no. After fixing the problem, you can re-issue a merge command. Please refrain from merging manually as it will most probably make the target branch red. |
@alexey-pelykh It is a good idea, but then we wont be able to autoapprove some leaves and not autoapprove other types. |
@jarroyomorales Setting per leave-type of course |
@alexey-pelykh Oh sorry, I thought you meant adding a field |
64d96ca
to
1cfc21f
Compare
Do we want to introduce this approach in this PR? |
OK, I can try, a migration script will be needed too, am i right? |
Yes, and I can help with that if needed |
@alexey-pelykh Can you review please? If it is okay i will go for the migration script |
And please squash to 1 commit |
03eeded
to
7025fbd
Compare
@alexey-pelykh Comments attended |
values.get('holiday_status_id') | ||
).auto_approve | ||
) | ||
auto_approve = leave_type.auto_approve_policy != 'no' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Extract as def _should_auto_approve
method
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't you think we can just delete the _get_auto_approve_on_creation()
function? It is used to disable the creation message in case the leave can be auto approved, which I dont undersand why. IMHO it is not necessary to do that
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we're self-approving, we don't want message about user approved own request being sent to himself, BUT there can be other watchers by default. Probably if "only user will receive message" then don't send it, or exclude self from followers, something like that. We can address that as separate PR
@@ -12,6 +12,13 @@ def _check_approval_update(self, state): | |||
return | |||
return super()._check_approval_update(state) | |||
|
|||
@api.multi | |||
def apply_auto_approve_policy(self): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use _
to make this method non-RPC-callable
def apply_auto_approve_policy(self): | ||
for record in self: | ||
policy = record.holiday_status_id.auto_approve_policy | ||
if (record.can_approve and policy == 'hr') or policy == 'all': |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reuse _should_auto_approve
?
if record._should_auto_approve():
record.sudo().action_approve()
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The whole function could be done in one line like this
self.filtered(lambda r: r._should_auto_approve()).sudo().action_approve()
but will we have possible future inheritance errors?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no, it would work normally
Direction is great, let's proceed to migration script as well 👍 |
7025fbd
to
11bb40a
Compare
@alexey-pelykh Migration done and works as expected. I am keeping the |
hr_holidays_leave_auto_approve/migrations/12.0.2.0.0/post-migration.py
Outdated
Show resolved
Hide resolved
values.get('holiday_status_id') | ||
).auto_approve | ||
) | ||
auto_approve = leave_type.auto_approve_policy != 'no' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we're self-approving, we don't want message about user approved own request being sent to himself, BUT there can be other watchers by default. Probably if "only user will receive message" then don't send it, or exclude self from followers, something like that. We can address that as separate PR
11bb40a
to
44f2984
Compare
/ocabot merge |
Hey, thanks for contributing! Proceeding to merge this for you. |
Congratulations, your PR was merged at 5fd1d77. Thanks a lot for contributing to OCA. ❤️ |
PR related to the discussion in #610 .
Added a new boolean field for leave types
auto_approve_all
which makes all leave requests of that type be automatically approved by the odoo bot.cc @alexey-pelykh @vmelnych @neilmitchell-goodson